Make Snapshot take a strategy trait so that users can write their own strategies
#82
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I just realized that since |
|
Hi!
I do appreciate the non-API-breaking way this was implemented. Is there a reason you're targetting the |
uint
suggested changes
Jul 4, 2024
| use super::{ChangeSet, SnapshotStrategy}; | ||
|
|
||
| /// A SnapshotStrategy that takes a snapshot only if at least the specified interval has passed. | ||
| #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] |
There was a problem hiding this comment.
Suggested change
| #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | |
| #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] |
clippy complains here
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a
SnapshotStrategytrait and adds a generic toSnapshot,SnapshotMap,SnapshotItem, andIndexedSnapshotMap. It also implements theSnapshotStrategytrait for theStrategyenum.This PR also adds a
IntervalStrategystruct which is a strategy that only stores data to the changelog if at least a certain number of blocks has passed.I tried my best to make these changes non-breaking and I think I succeeded. However, I think there are more changes that could be make to simplify and improve
Snapshot. For example, I found the "checkpointing" and the fnassert_checkpointedquite confusing and it seems this is only used in theStrategy::Selectedvariant. I couldn't figure out a way to move this logic into this variant without making breaking changes.I also found it a bit unintuitive that in
SnapshotMap, data added to the changelog use the current height and not the height at which they were originally saved to theprimaryMap. For the current purposes that I want to use SnapshotMap for it would make sense to use the height at which the data was originally saved, so that the changelog becomes a record of history rather than having a delay in the heights. Would you be interested in me adding an alternative implementation that accomplishes this? Trying to modify the current implementation would cause breaking changes.